[MINOR][SQL] Add missing functions for some options in SQLConf and use them where applicable by HyukjinKwon · Pull Request #14678 · apache/spark

HyukjinKwon · 2016-08-17T05:14:48Z

What changes were proposed in this pull request?

I first thought they are missing because they are kind of hidden options but it seems they are just missing.

For example, spark.sql.parquet.mergeSchema is documented in sql-programming-guide.md but this function is missing whereas many options such as spark.sql.join.preferSortMergeJoin are not documented but have its own function individually.

So, this PR suggests making them consistent by adding the missing functions for some options in SQLConf and use them where applicable, in order to make them more readable.

How was this patch tested?

Existing tests should cover this.

SparkQA · 2016-08-17T05:22:12Z

Test build #63898 has finished for PR 14678 at commit c959f3b.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2016-08-17T05:45:02Z

sql/core/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

      .intConf
      .createWithDefault(4000)

-  val PARTITION_DISCOVERY_ENABLED = SQLConfigBuilder("spark.sql.sources.partitionDiscovery.enabled")


It seems we always enable this and this option is not referenced anywhere(?). Is this intendedly not used?

It seems the usage was removed while refactoring.

Could you do me a favor to check which PR removed the usage?

It sounds like we still keep another SQLConf PARALLEL_PARTITION_DISCOVERY_THRESHOLD. However, the description of PARALLEL_PARTITION_DISCOVERY_THRESHOLD is not right after the refactoring. Could you update the descriptions too?

PR #11509 removes the usage of PARTITION_DISCOVERY_ENABLED. I think it is safe to remove this conf now.

HyukjinKwon · 2016-08-17T06:05:24Z

cc @rxin, Could you check if this make sense please?

rxin · 2016-08-17T06:58:52Z

Looks good, but I didn't look super carefully.

@gatorsmile do you have time to take a more careful look at this?

SparkQA · 2016-08-17T07:30:02Z

Test build #63899 has finished for PR 14678 at commit a57dd5e.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2016-08-17T14:31:56Z

Sure, will review it today. Thanks!

gatorsmile · 2016-08-17T16:37:03Z

Just searched the code and found more. Any reason why we do not change these?

ParquetFileFormat.scala:85:    if (conf.get(SQLConf.PARQUET_OUTPUT_COMMITTER_CLASS.key) == null) {
ParquetSchemaConverter.scala:66:    assumeBinaryIsString = conf.get(
ParquetSchemaConverter.scala:67:    assumeInt96IsTimestamp = conf.get(
ParquetSchemaConverter.scala:68:    writeLegacyParquetFormat = conf.get(
ParquetWriteSupport.scala:80:      assert(configuration.get(SQLConf.PARQUET_WRITE_LEGACY_FORMAT.key) != 
ParquetWriteSupport.scala:81:      configuration.get(SQLConf.PARQUET_WRITE_LEGACY_FORMAT.key).toBoolean

gatorsmile · 2016-08-17T16:42:42Z

nvm, I got the answer. We are unable to change them. Thanks!

gatorsmile · 2016-08-17T17:18:55Z

I checked the conf changes and the data types. It looks good to me. Only leave a minor comment on the conf description of one related parameter PARALLEL_PARTITION_DISCOVERY_THRESHOLD. Thanks!

HyukjinKwon · 2016-08-18T01:27:10Z

@gatorsmile Thanks for a very close look. I should've check the description of PARALLEL_PARTITION_DISCOVERY_THRESHOLD.

gatorsmile · 2016-08-18T01:59:33Z

LGTM pending tests

SparkQA · 2016-08-18T03:13:41Z

Test build #63959 has finished for PR 14678 at commit a26c08e.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2016-08-19T11:14:40Z

(@rxin cc you in case you just missed this)

HyukjinKwon · 2016-08-30T02:26:32Z

ping @rxin

HyukjinKwon · 2016-09-09T01:35:16Z

@cloud-fan Do you mind if I ask to take a look please?

cloud-fan · 2016-09-12T12:39:09Z

retest this please

cloud-fan · 2016-09-12T12:39:13Z

LGTM

SparkQA · 2016-09-12T17:20:05Z

Test build #65263 has finished for PR 14678 at commit a26c08e.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2016-09-14T17:34:39Z

thanks, merging to master!

rxin · 2016-09-16T02:31:01Z

Thanks but also FYI this is not "minor". We should have a JIRA ticket for changes like this in the future.

…e them where applicable ## What changes were proposed in this pull request? I first thought they are missing because they are kind of hidden options but it seems they are just missing. For example, `spark.sql.parquet.mergeSchema` is documented in [sql-programming-guide.md](https://github.com/apache/spark/blob/master/docs/sql-programming-guide.md) but this function is missing whereas many options such as `spark.sql.join.preferSortMergeJoin` are not documented but have its own function individually. So, this PR suggests making them consistent by adding the missing functions for some options in `SQLConf` and use them where applicable, in order to make them more readable. ## How was this patch tested? Existing tests should cover this. Author: hyukjinkwon <gurwls223@gmail.com> Closes apache#14678 from HyukjinKwon/sqlconf-cleanup.

HyukjinKwon added 2 commits August 17, 2016 13:50

Add missing functions for some options and use them where applicable

27fbbd9

Fix typos

c959f3b

Fix perentheses and remove unreferenced configuration for now

a57dd5e

HyukjinKwon reviewed Aug 17, 2016
View reviewed changes

Fix the description for PARALLEL_PARTITION_DISCOVERY_THRESHOLD

a26c08e

asfgit closed this in a79838b Sep 14, 2016

zsxwing mentioned this pull request Oct 12, 2016

[SPARK-15698][SQL][Streaming][Follw-up]Fix FileStream source and sink log get configuration issue #15173

Closed

HyukjinKwon deleted the sqlconf-cleanup branch January 2, 2018 03:39

Conversation

HyukjinKwon commented Aug 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Aug 17, 2016

Uh oh!

HyukjinKwon Aug 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Aug 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile Aug 17, 2016

Choose a reason for hiding this comment

Uh oh!

gatorsmile Aug 17, 2016

Choose a reason for hiding this comment

Uh oh!

gatorsmile Aug 17, 2016

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Aug 17, 2016

Uh oh!

rxin commented Aug 17, 2016

Uh oh!

SparkQA commented Aug 17, 2016

Uh oh!

gatorsmile commented Aug 17, 2016

Uh oh!

gatorsmile commented Aug 17, 2016

Uh oh!

gatorsmile commented Aug 17, 2016

Uh oh!

gatorsmile commented Aug 17, 2016

Uh oh!

HyukjinKwon commented Aug 18, 2016

Uh oh!

gatorsmile commented Aug 18, 2016

Uh oh!

SparkQA commented Aug 18, 2016

Uh oh!

HyukjinKwon commented Aug 19, 2016

Uh oh!

HyukjinKwon commented Aug 30, 2016

Uh oh!

HyukjinKwon commented Sep 9, 2016

Uh oh!

cloud-fan commented Sep 12, 2016

Uh oh!

cloud-fan commented Sep 12, 2016

Uh oh!

SparkQA commented Sep 12, 2016

Uh oh!

cloud-fan commented Sep 14, 2016

Uh oh!

rxin commented Sep 16, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

HyukjinKwon commented Aug 17, 2016 •

edited

Loading

HyukjinKwon Aug 17, 2016 •

edited

Loading

HyukjinKwon Aug 17, 2016 •

edited

Loading